-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/backend/strict null check #279
Conversation
Warning Rate Limit Exceeded@usatie has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 9 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent update aims to enhance various aspects of the backend to improve chat functionality. Changes include refining user validity checks, upgrading error handling, and enhancing code clarity. Additionally, optional chaining in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
- Naming consistency - Use match request instead of invite
95aeabf
to
f4f47c5
Compare
export class WsPublicUserEntity { | ||
constructor(user: WsPublicUserEntity) { | ||
this.id = user.id; | ||
this.name = user.name; | ||
this.avatarURL = user.avatarURL; | ||
} | ||
id: number; | ||
name: string; | ||
avatarURL?: string; | ||
avatarURL: string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor of WsPublicUserEntity
takes an instance of its own class as a parameter, which seems unusual for typical use cases. This pattern might be intended for cloning instances, but if that's not the case, it could lead to confusion or misuse. Consider revising the constructor's parameter to match the intended use case, such as accepting raw user data instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かに。まー引数にはPrismaのUserを取っておくか。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@usatie: 確かに、PrismaのUser
型をコンストラクタの引数として使用する提案は、初期のコメントで提起された懸念に直接対処します。この変更により、コンストラクタの目的がより明確になり、データベースからの生データを使ってエンティティを初期化するという一般的な使用例に合致します。この方向での修正がコードの明確性と使いやすさを向上させるでしょう。もし追加でサポートや説明が必要であれば、お知らせください。
- because the properties are required, it should not use PartialType
import { CreateUserOnRoomDto } from './create-UserOnRoom.dto'; | ||
|
||
export class UpdateUserOnRoomDto extends PartialType(CreateUserOnRoomDto) {} | ||
export class UpdateUserOnRoomDto extends CreateUserOnRoomDto {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directly extending CreateUserOnRoomDto
in UpdateUserOnRoomDto
without using PartialType
means all properties from the base DTO are required, which might not align with typical update operation needs where partial updates are common. Consider using PartialType
or an equivalent approach to make properties optional, enhancing flexibility for update operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateUserOnRoomがここでしか使われてなかったので存在する必要なかったので、CreateUserOnRoomの定義をそのままこっちに持ってきた。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@usatie: 了解しました。CreateUserOnRoomDto
が特定のコンテキストでのみ使用されているとのことで、その定義をUpdateUserOnRoomDto
に直接統合することは理にかなっています。この変更がPRの目的に沿っており、コードベースの柔軟性や保守性を損なわないことを確認することが重要です。変更がプロジェクト全体の品質と安定性向上に寄与することを期待しています。
New Learnings Added
User: usatie
PR: usatie/pong#279
File: backend/src/room/dto/update-UserOnRoom.dto.ts:0-0
Timestamp: 2024-02-21T23:40:40.188Z
Learning: `CreateUserOnRoomDto` is only used in the context of `UpdateUserOnRoomDto` and its definition has been merged directly into `UpdateUserOnRoomDto`.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure path-based instructions.
- Align with the prisma schema
- To align with prisma schema - But it is not desirable to have nullable password field
デプロイ先でこのエラーが起きていて、おや?と思ったところからstrictNullCheckがされていないことに気づいた
ONにしてみたら約60個エラーが出たので一応全部消しておいたというやつです